Skip to content

fix: oversubscription perps top movers#33150

Open
gambinish wants to merge 10 commits into
mainfrom
fix/oversubscription-perps-top-movers
Open

fix: oversubscription perps top movers#33150
gambinish wants to merge 10 commits into
mainfrom
fix/oversubscription-perps-top-movers

Conversation

@gambinish

@gambinish gambinish commented Jul 10, 2026

Copy link
Copy Markdown
Member

Description

Problem

The Now-tab perps movers strip subscribed to live prices for all ~329 markets and, on every ~3s push, rebuilt/re-sorted the full set and re-rendered — just to show the top 12 pills. Since Explore tabs never unmount, this kept running even on other tabs or after leaving Explore.

Why we keep the full-market subscription

Subscribing to only the 12 displayed symbols would break ranking: a market outside the top 12 can move into it, and we'd miss that price change. Correct live re-ordering requires watching every market.

The middle ground

The WebSocket already pushes all markets regardless of subscription — the real cost was the unconditional setState, the per-tick re-sort, and new object identities per tick. The new usePerpsLiveMovers hook merges every tick into a ref (no render per tick) and derives the displayed top-12 synchronously in render, so base-data loads and gainers/losers toggles reflect immediately with no empty frame. Live ticks only trigger a render when the displayed top-12 actually changes (via a symbol:formattedPercent fingerprint), reusing prior item references so unchanged pills skip re-render.

The full rank/filter/sort pass itself is also throttled to a configurable interval (recomputeIntervalMs, 10s on the Now tab): ticks arriving inside the interval accumulate losslessly in the ref, and at most one ranking pass runs per interval using the freshest data. Idle ticks cost only a cheap ref merge. On (re)subscribe — mount, tab resume, symbol-set change — the throttle anchor resets so the stream's cached snapshot surfaces promptly instead of waiting out a full interval.

On top of that, the subscription pauses entirely when not visible: an ExploreActiveTabContext tells PerpsBlock whether the Now tab is active, combined with useIsFocused() for leaving Explore. The same focus gating is applied to the What's Happening detail view's perps price hook. Disabling freezes the last-known data rather than clearing it, so nothing blanks out and it refreshes immediately on resume.

Changelog

CHANGELOG entry: improve performance on Perps Movers in explore

Related issues

Fixes:

Manual testing steps

Feature: my feature name

  Scenario: user [verb for user action]
    Given [describe expected initial app state]

    When user [verb for user action]
    Then [describe expected outcome]

Screenshots/Recordings

Before

After

Pre-merge author checklist

Performance checks (if applicable)

  • I've tested on Android
    • Ideally on a mid-range device; emulator is acceptable
  • I've tested with a power user scenario
    • Use these power-user SRPs to import wallets with many accounts and tokens
  • I've instrumented key operations with Sentry traces for production performance metrics

For performance guidelines and tooling, see the Performance Guide.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Low Risk
Performance-focused UI changes with broad unit test coverage; no auth, payments, or data-persistence changes. Residual risk is subtle ranking or stale UI if gating/fingerprint logic regresses.

Overview
Reduces Explore perps movers work by replacing inline usePerpsLivePrices + full-list sort in NowTab’s PerpsBlock with a new usePerpsLiveMovers hook that still watches all markets for correct top-12 ranking, but stores ticks in a ref and only re-renders when the visible movers fingerprint changes (with optional 10s batching, stable item references, and enabled to tear down the subscription and freeze the last slice).

Pauses subscriptions when not visible: ExploreActiveTabContext / useExploreActiveTab expose the active Explore tab without prop-drilling through every mounted tab; TrendingView tracks activeTab on tab change and wraps TabsList. PerpsBlock enables live movers only when the screen is focused and the Now tab is active. useWhatsHappeningAssetPrices passes an empty symbol list while unfocused so perps prices pause without clearing displayed values.

Tests cover the new hook, context, tab gating, and focus behavior.

Reviewed by Cursor Bugbot for commit ee983bd. Bugbot is set up for automated code reviews on this repo. Configure here.

@mm-token-exchange-service mm-token-exchange-service Bot added the team-perps Perps team label Jul 10, 2026
@mm-token-exchange-service

mm-token-exchange-service Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

PR template — items to address before "Ready for review"

Warnings — informational, address before merging:

  • Description section is empty. Describe what changed and why.
  • Related issues section is empty. Add Fixes: #123 / Closes: <URL> / Refs: <Jira key>, or write a short rationale after the colon.
  • Manual testing steps still contain template content (the Gherkin example title or a [...] placeholder). Replace with real steps, or write N/A — <reason>.
  • Screenshots/Recordings section is empty. Add an image/video for user-facing changes, logs/console output for non-user-facing changes, or write N/A if no evidence is applicable.
  • Pre-merge author checklist has unchecked items (e.g. "I've followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards."). Every box must be consciously checked — see docs/readme/ready-for-review.md.

See docs/readme/ready-for-review.md for the full Definition of Ready for Review.

@gambinish gambinish marked this pull request as ready for review July 13, 2026 21:14
@gambinish gambinish requested a review from a team as a code owner July 13, 2026 21:14
Comment thread app/components/Views/TrendingView/feeds/perps/usePerpsLiveMovers.ts Outdated
@github-actions github-actions Bot added the risk:low AI analysis: low risk label Jul 13, 2026
@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

🧪 Flaky unit test detection

Run history flaky detection

View recent run history

Historical failure rate is a hint, not proof — review each suggestion in context. See the flaky-test-detection skill for the full pattern reference and manual audit workflow.

Failures / runs sampled per window:

File 7d 15d 30d
app/components/Views/TrendingView/feeds/perps/usePerpsLiveMovers.test.ts 0/62 0/190 0/364
app/components/Views/TrendingView/tabs/NowTab.test.tsx 0/62 0/190 0/364

AI-detected flaky patterns

app/components/Views/TrendingView/feeds/perps/usePerpsLiveMovers.test.ts

  • J8 — jest.useFakeTimers() combined with waitFor (polling conflict / missing clearAllTimers) (high)
    • In every test inside the recomputeIntervalMs describe block, jest.useFakeTimers() is called mid-test (after an initial await waitFor(...) that resolves under real timers). The afterEach only calls jest.useRealTimers() but never jest.clearAllTimers(). Two risks arise:
  1. Pending timer leak on test failure: If a test throws after jest.useFakeTimers() but before jest.advanceTimersByTime(...) fires the pending 10 s recompute timer, that timer remains in the fake queue. jest.useRealTimers() in afterEach switches the clock back but does NOT drain the fake queue. Depending on the Jest version, those pending callbacks can fire unexpectedly in the next test, causing order-dependent failures.

  2. Mid-test timer activation: Calling jest.useFakeTimers() inside the test body (rather than in beforeEach) means the fake-timer state is not guaranteed to be clean at the start of each test. Moving the call to beforeEach (and pairing it with jest.clearAllTimers() in afterEach) makes the timer lifecycle explicit and symmetric.

The fix moves jest.useFakeTimers() to beforeEach and adds jest.clearAllTimers() before jest.useRealTimers() in afterEach. Because the initial await waitFor(...) in each test must run under real timers, the tests are restructured to call jest.useRealTimers() for that initial setup phase and then switch to fake timers — or, more cleanly, the initial subscription/mount assertion is extracted into a shared beforeEach that runs before fake timers are installed.

  • Suggested fix in app/components/Views/TrendingView/feeds/perps/usePerpsLiveMovers.test.ts:340:
    -  describe('recomputeIntervalMs', () => {
    -    afterEach(() => {
    -      jest.useRealTimers();
    -    });
    -
    -    it('batches multiple ticks inside the interval into a single recompute using the freshest percents', async () => {
    -      // ...
    -      await waitFor(() => {
    -        expect(result.current.map((item) => item.market.symbol)).toEqual([
    -          'HIGH_GAINER',
    -          'LOW_GAINER',
    -        ]);
    -      });
    -
    -      jest.useFakeTimers();
    -      // ...
    -    });
    +  describe('recomputeIntervalMs', () => {
    +    beforeEach(() => {
    +      jest.useFakeTimers();
    +    });
    +
    +    afterEach(() => {
    +      jest.clearAllTimers();
    +      jest.useRealTimers();
    +    });
    +
    +    // In each test, replace the initial `await waitFor(...)` block that
    +    // previously ran under real timers with an `act`-based flush, since
    +    // fake timers are now active from the start:
    +    //
    +    //   Before:
    +    //     await waitFor(() => {
    +    //       expect(result.current.map(...)).toEqual([...]);
    +    //     });
    +    //     jest.useFakeTimers(); // ← remove this line from every test
    +    //
    +    //   After:
    +    //     await act(async () => { jest.runAllTimersAsync(); });
    +    //     expect(result.current.map(...)).toEqual([...]);
    +    //
    +    // This keeps the timer lifecycle symmetric (fake timers always active
    +    // inside the describe block) and prevents pending-timer leaks on
    +    // test failure.
  • J9 — Module-level mutable let bindings not reset in beforeEach (high)
    • The module-level mockSubscribeToSymbols jest.fn() is shared across all tests. The beforeEach correctly calls jest.clearAllMocks(), which resets call counts and clears mock implementations set via mockReturnValue/mockImplementation. However, jest.clearAllMocks() does NOT restore the default implementation — it only clears call history and removes per-call return values. Tests that call mockSubscribeToSymbols.mockImplementation(...) or mockSubscribeToSymbols.mockReturnValue(...) set a persistent implementation that clearAllMocks will clear, but the inner usePerpsStream factory function (the outer jest.fn(() => ({ prices: { subscribeToSymbols: mockSubscribeToSymbols } }))) is also a jest.fn() whose implementation is set at mock-definition time and is never reset.

More concretely: usePerpsStream is a jest.fn() whose factory is defined once in jest.mock(...). If any test calls jest.mocked(usePerpsStream).mockImplementation(...) to override it, that override persists into subsequent tests because clearAllMocks only clears call counts, not implementations (that requires resetAllMocks). The current beforeEach uses clearAllMocks, which is insufficient to guard against implementation bleed if a future test overrides usePerpsStream directly.

The safest fix is to change jest.clearAllMocks() to jest.resetAllMocks() in beforeEach and re-apply the default mockSubscribeToSymbols return value there, so every test starts from a known state regardless of what the previous test did.

  • Suggested fix in app/components/Views/TrendingView/feeds/perps/usePerpsLiveMovers.test.ts:13:
    -const mockSubscribeToSymbols = jest.fn();
    -
    -jest.mock('../../../../UI/Perps/providers/PerpsStreamManager', () => ({
    -  usePerpsStream: jest.fn(() => ({
    -    prices: {
    -      subscribeToSymbols: mockSubscribeToSymbols,
    -    },
    -  })),
    -}));
    +  beforeEach(() => {
    +    jest.resetAllMocks(); // resets implementations AND call counts
    +    // Re-apply the default implementation so every test starts from a
    +    // known state without relying on the module-factory default.
    +    jest.mocked(
    +      require('../../../../UI/Perps/providers/PerpsStreamManager').usePerpsStream,
    +    ).mockReturnValue({
    +      prices: {
    +        subscribeToSymbols: mockSubscribeToSymbols,
    +      },
    +    });
    +  });

app/components/Views/TrendingView/tabs/NowTab.test.tsx

  • J3 — Missing jest.clearAllMocks() / jest.resetAllMocks() in beforeEach (high)
    • jest.clearAllMocks() is called inside the arrangeMocks() helper rather than in a beforeEach hook. Every describe block delegates to arrangeMocks() (via arrangePerpsMoversMocks(), arrangeCryptoMoversMocks(), etc.), so in the current test suite this works. However, this is a fragile pattern: any future test that is added without calling arrangeMocks() — or that calls it after some side-effectful setup — will inherit stale call counts and mock implementations from the previous test. Jest runs tests in file order by default, but --randomize or parallel sharding can expose the ordering dependency. The fix is to move jest.clearAllMocks() (and the default mock implementations) into a top-level beforeEach so isolation is guaranteed unconditionally.
    • Suggested fix in app/components/Views/TrendingView/tabs/NowTab.test.tsx:248:
      -const arrangeMocks = () => {
      -  jest.clearAllMocks();
      -  mockUseIsFocused.mockReturnValue(true);
      -
      -  const mockUseSelector = useSelector as jest.MockedFunction<
      -    typeof useSelector
      -  >;
      -  const mockUseTokensFeed = useTokensFeed as jest.MockedFunction<
      -    typeof useTokensFeed
      -  >;
      -
      -  mockUseSelector.mockImplementation(
      -    createMockSelectorImpl({
      -      perpsEnabled: false,
      -      predictEnabled: false,
      -      whatsHappeningEnabled: false,
      -    }),
      -  );
      -  // ... (rest of mock setup)
      -};
      +// Add a top-level beforeEach that always clears mocks and restores defaults.
      +// Keep arrangeMocks() for the per-describe overrides, but remove the
      +// jest.clearAllMocks() call from inside it.
      +
      +beforeEach(() => {
      +  jest.clearAllMocks();
      +  // Restore module-level mock defaults so every test starts from a known state.
      +  mockUseIsFocused.mockReturnValue(true);
      +  mockSubscribeToSymbols.mockReturnValue(jest.fn());
      +  mockWhatsHappeningRefresh.mockReset();
      +  mockUseWhatsHappening.mockReturnValue({
      +    items: [{ id: 'trend-0' }],
      +    isLoading: false,
      +    error: null,
      +    refresh: mockWhatsHappeningRefresh,
      +  });
      +});
      +
      +// Inside arrangeMocks(), remove the jest.clearAllMocks() line:
      +const arrangeMocks = () => {
      +  // jest.clearAllMocks(); ← remove this line
      +  const mockUseSelector = useSelector as jest.MockedFunction<typeof useSelector>;
      +  // ... rest of per-describe setup unchanged
      +};
  • J9 — Module-level mutable let bindings not reset in beforeEach (high)
    • All module-level mock functions (mockNavigate, mockUseIsFocused, mockSubscribeToSymbols, mockWhatsHappeningRefresh, mockUseWhatsHappening, mockNavigateToPerpsMarketList, mockPredictionsCarouselSection) are declared at module scope and their call history / implementations are only reset inside arrangeMocks(), which is called manually per test. If a test is skipped, fails early, or is added without calling arrangeMocks(), the accumulated call counts and overridden implementations from the previous test bleed into the next one. For example, mockUseIsFocused is set to false in one 'live movers subscription gating' test and is never explicitly restored to true unless arrangeMocks() is called next. Moving the reset to beforeEach eliminates this ordering dependency.
    • Suggested fix in app/components/Views/TrendingView/tabs/NowTab.test.tsx:5:
      -const mockNavigate = jest.fn();
      -const mockUseIsFocused = jest.fn(() => true);
      -
      -// ...
      -
      -const mockSubscribeToSymbols = jest.fn(() => jest.fn());
      -
      -// ...
      -
      -const mockWhatsHappeningRefresh = jest.fn();
      -const mockUseWhatsHappening = jest.fn(() => ({
      -  items: [] as { id: string }[],
      -  isLoading: false,
      -  error: null as string | null,
      -  refresh: mockWhatsHappeningRefresh,
      -}));
      -
      -// ...
      -
      -const mockNavigateToPerpsMarketList = jest.fn();
      -const mockPredictionsCarouselSection = jest.fn(/* ... */);
      +// At the top level of the describe block (or file), add:
      +beforeEach(() => {
      +  jest.clearAllMocks();
      +  // Re-apply default implementations for every module-level mock:
      +  mockUseIsFocused.mockReturnValue(true);
      +  mockSubscribeToSymbols.mockImplementation(() => jest.fn());
      +  mockUsePerpsFeed.mockReturnValue({
      +    data: [],
      +    isLoading: false,
      +    refetch: jest.fn(),
      +    defaultSortOptionId: 'priceChange' as const,
      +  });
      +  mockUseWhatsHappening.mockReturnValue({
      +    items: [],
      +    isLoading: false,
      +    error: null,
      +    refresh: mockWhatsHappeningRefresh,
      +  });
      +});

This check is informational only and does not block merging.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.80519% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.32%. Comparing base (9aad058) to head (eb320cb).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...ews/TrendingView/feeds/perps/usePerpsLiveMovers.ts 93.44% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #33150      +/-   ##
==========================================
+ Coverage   84.31%   84.32%   +0.01%     
==========================================
  Files        6080     6084       +4     
  Lines      162378   162522     +144     
  Branches    39577    39616      +39     
==========================================
+ Hits       136905   137050     +145     
+ Misses      16021    16010      -11     
- Partials     9452     9462      +10     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot added size-XL and removed size-L labels Jul 13, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 9f36d83. Configure here.

Comment thread app/components/Views/TrendingView/feeds/perps/usePerpsLiveMovers.ts
@github-actions github-actions Bot added risk:medium AI analysis: medium risk and removed risk:low AI analysis: low risk labels Jul 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeWalletPlatform, SmokePerps, SmokeConfirmations
  • Selected Performance tags: @PerformancePreps
  • Risk Level: medium
  • AI Confidence: 85%
click to see 🤖 AI reasoning details

E2E Test Selection:
The PR introduces:

  1. A new ExploreActiveTabContext to track the active Explore tab without prop drilling
  2. Wraps TrendingView's TabsList with the new provider
  3. Refactors NowTab's PerpsBlock to use the new context + useIsFocused to pause WebSocket subscriptions when the screen/tab is not active
  4. Introduces usePerpsLiveMovers hook for efficient live ranking of perps markets
  5. Adds focus-gating to useWhatsHappeningAssetPrices to pause subscriptions when unfocused

These changes directly affect:

  • SmokeWalletPlatform: TrendingView is the Trending/Explore tab, and the tab switching logic and context provider wrapping the entire TabsList is modified. Any regression in tab switching or context propagation would break the Trending experience.
  • SmokePerps: The PerpsBlock in NowTab is significantly refactored — the live movers subscription logic is replaced with a new hook, and the subscription is now paused based on focus/tab state. This could affect the Perps section display in Trending.
  • SmokeConfirmations: Required per SmokePerps tag description (Add Funds deposits are on-chain transactions).

The changes are well-contained to the TrendingView/Perps area with no impact on core wallet infrastructure, navigation, or other flows. Risk is medium because the WebSocket subscription management changes could affect live data display in the Perps section.

Performance Test Selection:
The PR introduces significant changes to how the Perps live price WebSocket subscription is managed in the TrendingView. The new usePerpsLiveMovers hook adds throttling, fingerprint-based change detection, and focus-gating to minimize re-renders. These optimizations directly affect the performance of the Perps section in the Trending tab (loading, live price updates, rendering). The @PerformancePreps tag covers perps market loading, position management, add funds flow, and order execution — all of which could be affected by the subscription management changes.

View GitHub Actions results

@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown
Contributor

⚡ Performance Test Results

ℹ️ Performance test results are currently non-blocking and will not block this PR.

All tests passed · 2 tests · 1 device

📱 Devices tested (1)

Android: Google Pixel 8 Pro (v14.0)

✅ Passed Tests (2)
Test Platform Device Duration Team Recording
Perps add funds Android Google Pixel 8 Pro (v14.0) 10.14s @mm-perps-engineering-team 📹 Watch
Perps open position and close it Android Google Pixel 8 Pro (v14.0) 15.15s @mm-perps-engineering-team 📹 Watch

Branch: fix/oversubscription-perps-top-movers · Build: Normal · Commit: e2207c2 · View full run

@geositta geositta left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work overall addressing the root cause. Before approving, please fix the stale market data path. I also recommend narrowing the active tab update so this performance change does not rerender every loaded Explore feed.

previous &&
previous.market.change24hPercent === market.change24hPercent
) {
return previous;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the updated feed item when items changes. If a REST refresh changes maxLeverage or another market field while change24hPercent stays the same, this branch keeps the old market data. The claimed render reduction also does not occur at the current call site because PerpsPillItem receives a new onCardPress function on each render, causing React.memo to render it again.

Please retain old items only for WebSocket updates where the complete feed item is unchanged, and add a test where the percentage remains constant while another market field changes.

...(source ? { source } : {}),
});
previousTabRef.current = destinationTab;
setActiveTab(destinationTab);

@geositta geositta Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This state update rerenders ExploreFeed on every tab switch. useExploreRefresh() returns a new object, which recreates all six tab elements and causes every loaded feed to rerender - not only the component using the context. This adds rendering work during tab changes despite the context being intended to isolate active tab updates. Please move the active tab state into a smaller component or preserve stable tab content references when the active tab changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:medium AI analysis: medium risk size-XL team-perps Perps team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants